Encode clientId and clientSecret for OpaqueTokenIntrospector and ReactiveOpaqueTokenIntrospector
#16008
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
jzheaux
added
status: blocked
There was a problem hiding this comment.
@ngocnhan-tran1996 thanks for your patience while we got ready for 6.5 development. I've left my feedback inline.
| @@ -82,8 +84,10 @@ public NimbusOpaqueTokenIntrospector(String introspectionUri, String clientId, S | |||
| Assert.notNull(clientId, "clientId cannot be null"); | |||
| Assert.notNull(clientSecret, "clientSecret cannot be null"); | |||
| this.requestEntityConverter = this.defaultRequestEntityConverter(URI.create(introspectionUri)); | |||
| String encodeClientId = URLEncoder.encode(clientId, StandardCharsets.UTF_8); | |||
There was a problem hiding this comment.
We tend to leave deprecated classes as-is, when we are able. This is a subtle encouragement to move onto the new class. Given that, will you please leave NimbusOpaqueTokenIntrospector and its reactive counterpart out of this PR?
| @@ -84,8 +86,10 @@ public SpringOpaqueTokenIntrospector(String introspectionUri, String clientId, S | |||
| Assert.notNull(clientId, "clientId cannot be null"); | |||
| Assert.notNull(clientSecret, "clientSecret cannot be null"); | |||
| this.requestEntityConverter = this.defaultRequestEntityConverter(URI.create(introspectionUri)); | |||
| String encodeClientId = URLEncoder.encode(clientId, StandardCharsets.UTF_8); | |||
There was a problem hiding this comment.
Thinking about this more, even though the scenario is unlikely, we should remain passive with the constructor.
Instead, I believe there is value in adding a builder and deprecating this constructor in this and the reactive class.
public static Builder withIntrospectionUri(String uri)The builder's JavaDoc should clearly indicate that the parameters should be unencoded and that the builder will encode them. The builder will ultimately call the RestOperations constructor.
So that we can move OAuth2ResourceConfigurer and OAuth2ResourceServerSpect to use the Builder and remain passive, it should decode the values it receives in the DSL. We'll switch this behavior in 7.
Also in that case, we'd add JavaDoc to the depreceated constructors that states that they do not encode the values and add tests to confirm that.
ngocnhan-tran1996
force-pushed
the
encode-client-before-calling-basic-auth
branch
3 times, most recently
from
9a239bc to
c1f2f7d
Compare
There was a problem hiding this comment.
Thanks, @ngocnhan-tran1996! I've sent you some of my feedback, mostly about Security's code conventions. Also, if you are able, please also update the documentation to use the builder instead of the now-deprecated constructor.
| * @author Ngoc Nhan | ||
| * @since 6.5 | ||
| */ | ||
| public static final class SpringOpaqueTokenIntrospectorBuilder { |
There was a problem hiding this comment.
I think you can just call this Builder as the fully-qualified name already includes SpringOpaqueTokenIntrospector.
| * @return the {@link SpringOpaqueTokenIntrospectorBuilder} | ||
| * @since 6.5 | ||
| */ | ||
| public static SpringOpaqueTokenIntrospectorBuilder withIntrospectionUri(String introspectionUri) { |
There was a problem hiding this comment.
As a matter of convention, please move this out of the inner class and into SpringOpaqueTokenIntrospector.
| * @return the {@link SpringOpaqueTokenIntrospector} | ||
| * @since 6.5 | ||
| */ | ||
| public SpringOpaqueTokenIntrospector introspectionClientCredentials(String clientId, String clientSecret) { |
There was a problem hiding this comment.
Please separate these, again as a matter of convention. For additional brevity, can you please name them clientId and clientSecret.
| * @return the {@link SpringOpaqueTokenIntrospector} | ||
| * @since 6.5 | ||
| */ | ||
| public SpringOpaqueTokenIntrospector introspectionClientCredentials(String clientId, String clientSecret) { |
There was a problem hiding this comment.
It's more common for a builder to have a build() method. That will make it more obvious to a coder which method they call when they are done. Can you so that a build() method is what calls the constructor?
| * @return the {@link SpringOpaqueTokenIntrospector} | ||
| * @since 6.5 | ||
| */ | ||
| public SpringOpaqueTokenIntrospector withRestOperations(RestOperations restOperations) { |
There was a problem hiding this comment.
Let's leave this out for now. If they have a RestOperations, they can simply use the RestOperations constructor. Since they at that point need to add their own interceptor, I trust that they know what they are doing regarding client id and secret.
| * @return the {@link SpringOpaqueTokenIntrospector} | ||
| * @since 6.5 | ||
| */ | ||
| public SpringOpaqueTokenIntrospector introspectionEncodeClientCredentials(String clientId, String clientSecret, |
There was a problem hiding this comment.
Please leave these convenience methods out for now. I think we can start with clientId, clientSecret, and build.
ngocnhan-tran1996
force-pushed
the
encode-client-before-calling-basic-auth
branch
from
b55d6be to
116b041
Compare
Closes gh-15988